fix(transforms): compile MDX before parsing its imports (depends on #2999) - #3009
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6df7b936f3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "server", | ||
| ); | ||
| parseSource = compiled.compiledCode; |
There was a problem hiding this comment.
Handle server-rewritten MDX imports
When an MDX file imports a project file with a relative specifier, the real content compiler invoked here with the server target rewrites ./Child or ../Child to a file://... specifier before parseSource is lexed. The loop below only records ./, ../, and @/ imports, so those relative MDX dependencies no longer enter parseResult.imports; SSRModuleLoader then skips the recursive transform and cannot rewrite them to cache temp modules, leaving the compiled MDX to import raw project files or miss dependency errors. Please preserve the original specifiers for dependency parsing or normalize file:// imports back to local project paths.
Useful? React with 👍 / 👎.
016c9ae to
8b8ce7e
Compare
6df7b93 to
b6d4ac1
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Score: 86/100
Compiling MDX/Markdown before dependency scanning matches the main transform pipeline and fixes the raw-MDX-to-esbuild failure. The targeted tests cover MDX with imports, MDX without imports, TSX, and CSS/JSON short-circuits.
Not merge-ready yet:
- This is stacked on lower open PRs, including blocked/type-failing PRs.
- The PR status rollup only shows CLA, not full CI.
Watch item: import discovery now depends on the full MDX content compiler in this utility path. That is probably the right semantic match, but it increases coupling and runtime cost, so it should remain intentional.
8b8ce7e to
69e526f
Compare
b6d4ac1 to
4566899
Compare
69e526f to
7506907
Compare
4566899 to
7d862f1
Compare
7d862f1 to
0d6621f
Compare
00674af to
d3848ba
Compare
0d6621f to
1c906fa
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Follow-up approval after #2999 merged and the stack was rebased onto main.
Score: 91/100.
Rationale: MDX import compilation ordering is scoped and covered by the rebased stack regression pass.
Next step: merge after lower stack PRs and refreshed checks are green.
eed4452 to
6a0d2f6
Compare
4a473a3 to
ee402cc
Compare
6a0d2f6 to
82c90c5
Compare
ee402cc to
edcb779
Compare
82c90c5 to
96c3a4d
Compare
edcb779 to
d054352
Compare
27ac9dd to
e42febe
Compare
7f53c76 to
94cb29b
Compare
59203c4 to
2b05d70
Compare
94cb29b to
36d7973
Compare
2b05d70 to
1649f59
Compare
36d7973 to
17b4f24
Compare
1649f59 to
cd3180c
Compare
17b4f24 to
7a3df0c
Compare
cd3180c to
a70330e
Compare
7a3df0c to
daf8bbb
Compare
a70330e to
7cecf2d
Compare
4df9e0e to
57e5df1
Compare
7cecf2d to
2b79b86
Compare
MDX source is not JSX, so parsing raw .mdx through the JSX loader reported existing dependencies as missing and dropped server-rewritten file:// imports. Constraint: Unit tests should keep Deno leak checks enabled and close the shared esbuild service instead of adding sanitizer opt-outs. Rejected: Parsing raw MDX as JSX | it fails before import analysis can see the compiled module imports. Confidence: high Scope-risk: moderate Directive: Keep MDX import analysis aligned with the content compiler target that rewrites imports for SSR. Tested: deno test --allow-all src/transforms/esm/import-parser.test.ts Tested: deno task lint:sanitizer-baseline Tested: deno task lint Not-tested: full repository CI locally
57e5df1 to
9923c98
Compare
kwakayama
left a comment
There was a problem hiding this comment.
Approval after rebasing onto current main and removing sanitizer opt-outs from the import-parser regression test.
Score: 92/100.
Real bug: raw .mdx is not JSX, so parsing it directly through the JSX/esbuild path can fail before dependency analysis sees the imports. The fix compiles MDX first and then parses the compiled module, including server-rewritten file:// imports for relative components and CSS.
Quality note: the test now keeps Deno leak checks enabled and stops the shared esbuild service instead of raising the sanitizer baseline.
Verification:
- deno test --allow-all src/transforms/esm/import-parser.test.ts
- deno task lint:sanitizer-baseline
- deno task lint
- deno test --allow-all src/transforms/esm/import-parser.test.ts src/transforms/esm/specifier-resolver.test.ts src/rendering/orchestrator/module-loader/esm-rewriter.test.ts
Next step: wait for refreshed GitHub checks, then merge when green.
Summary
A
.tsxpage importing an.mdxcomponent 500'd with a message that sent you looking for a file that was sitting right there:The file was found and read. The dependency scan of it failed.
parseLocalImportshands source to esbuild to strip types before lexing imports, choosing the loader from the file extension — and.mdxmaps to thejsxloader. Markdown prose is not JSX, so esbuild rejected byte one, and the resulting exception was reported as a missing dependency.That also explains the asymmetry the reproducer noticed — MDX importing MDX works fine, because MDX pages are compiled through the parse stage first; only this dependency-scanning path skipped it.
MDX and Markdown are now compiled to JSX first, exactly as the transform pipeline's parse stage does, and the imports are read out of that.
.cssand.jsonalready short-circuited above this point;.mdxsimply had no equivalent handling.Reproduction
r-tsx-imports-mdx.tsxsrc/transforms/esm/import-parser.test.ts(new — this module had no direct coverage)Test evidence
The MDX cases register a stub
ContentProcessor(the MDX extension is not loaded in unit tests) and assert both that the imports are found and that the content compiler was invoked — so the routing of.mdxaway from raw esbuild is what is actually under test.Wider suite:
deno task test:unit→ 2525 passed | 0 failed.SSR evidence
The MDX renders inline, which is what the route was written to check.
Client evidence
All six MDX routes in the matrix (
n,o,p,q,r,s) now pass the hydration sweep.Related
Chain: this PR is part of a 13-PR chain fixing the bugs catalogued in
veryfront-router-testing.
Its base is the previous PR in the chain, so the diff shows only this fix.
The root of the chain is #2999 (
fix/ssr-lazy-import-graceful-degrade) — merge #2999 first, thenrebase the chain onto
main.Regression gate:
deno task test:unit→ 2525 passed | 0 failed;deno task lint,deno task fmt:checkanddeno task typecheckall clean. The reproducer's full 56-routematrix (
ROUTES.txt+sweep.sh) was re-run after every fix: 7 routes improved, 0 regressed.A 46-route Chromium hydration sweep (
client-sweep.mjs) backs the client-side claims.